Conversation
eladalon1983
left a comment
eladalon1983
left a comment
There was a problem hiding this comment.
Generally LGTM, but some suggestions.
| <dfn>capture-session</dfn>.</p> | ||
| <dfn>capture-session</dfn>, with the associated | ||
| [=capture-source-set=], <var>sources</var>.</p> | ||
| </li> |
There was a problem hiding this comment.
Should the part below, which dealt with controller.[[Source]], be updated to reference the video source from capture-source-set rather than (as it does now) the "video track's source"?
(GitHub didn't let me attach this comment in the desired location.)
| The user agent MUST NOT change the [=display surface=] | ||
| associated with a [=capture-session=] unless the user has | ||
| explicitly indicated that they want this change to be | ||
| performed by interacting with user agent or operating system. |
There was a problem hiding this comment.
nit: Could you make editorial changes here so as to discourage anyone from misreading "by interacting" as binding to the wrong part of the sentence? Maybe "...performed. Users can do this by interacting with..."
tovepet
force-pushed
the
display-surface-change-255
branch
2 times, most recently
from
34aa232 to
ab47c8a
Compare
tovepet
force-pushed
the
display-surface-change-255
branch
from
ab47c8a to
7c6434f
Compare
|
|
||
|
|
||
| <p> | ||
| The user agent MUST NOT change the [=display surface=] |
There was a problem hiding this comment.
I don't think we can validate this, I would tend to reword so that we do not use MUST NOT.
| explicitly indicated that they want this change to be | ||
| performed by interacting with user agent or operating system. | ||
|
|
||
| If the user indicates a change of [=display surface=], the |
There was a problem hiding this comment.
We probably need to specify the order of the callback execution and the configurationchange event at MediaStreamTrack.
I would tend to have the event be executed after the callback.
|
|
||
| <li> | ||
| <p>Stop delivering frames from all sources in the | ||
| [=capture-source-set=] associated with the [=capture-session=]. |
There was a problem hiding this comment.
If audio source is not changing, should we allow audio to flow?
|
|
||
|
|
||
| <p> | ||
| <var>newSources</var> MUST fulfill the same requirements as the |
There was a problem hiding this comment.
It is not really clear to me what this MUST is about.
|
|
||
| <li> | ||
| <p>If <var>controller.{{CaptureController/[[DisplaySurfaceChangeCallback]]}}</var> | ||
| is null, abort these steps.</p> |
There was a problem hiding this comment.
I think we should do this check later inside a task queued since CaptureController/[DisplaySurfaceChangeCallback] can be changed at any point in time by JS, so the checks should be done within the context event loop.
|
|
||
|
|
||
| <li> | ||
| <p>Queue a task to:</p> |
There was a problem hiding this comment.
Will it have a link to the definition?
We should probably start using a task source as well.
| <li> | ||
| <p>Run the [=ApplyConstraints algorithm=] on all tracks in | ||
| <var>stream</var> with the appropriate constraints. Should | ||
| this fail, abort these steps.</p> |
There was a problem hiding this comment.
Why should we fail, user wants to switch, it is ok for the web page to update its constraints as part of the switch.
|
|
||
| <li> | ||
| <p>Queue tasks to end all tracks connected to sources in the | ||
| [=capture-source-set=] associated with the [=capture-session=]. |
There was a problem hiding this comment.
I think the default should be to leave the tracks alive, the future option being for the web page to ask for tracks to be ended.
The principle is that a web page can easily implement ending of the tracks, but it cannot implement the reverse.
|
|
||
|
|
||
| <li> | ||
| <p>If [=this=].{{CaptureController/[[IsBound]]}} is |
There was a problem hiding this comment.
It seems restrictive to disallow setting the callback more dynamically.
Why should we add this restriction?
| <p>If | ||
| [=this=].{{CaptureController/[[DisplaySurfaceChangeCallback]]}} | ||
| is not <code>null</code>, [=exception/throw=] an | ||
| "{{InvalidStateError}}" {{DOMException}}.</p> |
There was a problem hiding this comment.
I am not sure why we should not allow to overwrite the callback either.
There was a problem hiding this comment.
This seems different from w3c/mediacapture-screen-share-extensions#4 (comment):
Something like
captureController.processSourceSwitch(stream => { ... });orcaptureController.processSourceSwitch(null);I’m fine with this.
4 participants
Fixes w3c/mediacapture-screen-share-extensions#4.
Preview | Diff